Skip to content

fix: prevent NRE on transport level gRPC errors#405

Merged
w1am merged 1 commit intomasterfrom
fix/handle-null-rpc-status
Apr 14, 2026
Merged

fix: prevent NRE on transport level gRPC errors#405
w1am merged 1 commit intomasterfrom
fix/handle-null-rpc-status

Conversation

@w1am
Copy link
Copy Markdown
Contributor

@w1am w1am commented Apr 13, 2026

Summary

GetRpcStatus() can return null for transport-level gRPC errors (connection drops, timeouts), but the code assumed it never would. This caused a NullReferenceException that masked the actual error. Now the original RpcException is surfaced instead.

Copilot AI review requested due to automatic review settings April 13, 2026 11:15
@qodo-code-review
Copy link
Copy Markdown

Review Summary by Qodo

Handle null GetRpcStatus() to prevent NRE on transport errors

🐞 Bug fix

Grey Divider

Walkthroughs

Description
• Replace null-forgiving operator with null-coalescing throw in exception factories
• Use null-conditional operator in MultiAppend catch block for safe status access
• Prevent NullReferenceException on transport-level gRPC errors
• Surface original RpcException instead of masking with NRE
Diagram
flowchart LR
  A["RpcException with null Status"] -->|GetRpcStatus()!| B["NullReferenceException"]
  A -->|GetRpcStatus() ?? throw ex| C["Original RpcException surfaced"]
  D["MultiAppend catch block"] -->|ex.GetRpcStatus()!| E["NRE on null status"]
  D -->|ex.GetRpcStatus()?| F["Safe null-conditional access"]
Loading

Grey Divider

File Changes

1. src/KurrentDB.Client/Core/Exceptions/AccessDeniedException.cs 🐞 Bug fix +2/-1

Replace null-forgiving with null-coalescing throw

• Replace GetRpcStatus()! with GetRpcStatus() ?? throw ex in FromRpcException factory method
• Ensures original RpcException is thrown when status is null

src/KurrentDB.Client/Core/Exceptions/AccessDeniedException.cs


2. src/KurrentDB.Client/Core/Exceptions/AppendTransactionMaxSizeExceededException.cs 🐞 Bug fix +2/-1

Replace null-forgiving with null-coalescing throw

• Replace GetRpcStatus()! with GetRpcStatus() ?? throw ex in FromRpcException factory method
• Prevents NRE by throwing original exception when status is unavailable

src/KurrentDB.Client/Core/Exceptions/AppendTransactionMaxSizeExceededException.cs


3. src/KurrentDB.Client/Core/Exceptions/NotLeaderException.cs 🐞 Bug fix +2/-1

Replace null-forgiving with null-coalescing throw

• Replace GetRpcStatus()! with GetRpcStatus() ?? throw ex in FromRpcException factory method
• Handles null status gracefully by rethrowing original exception

src/KurrentDB.Client/Core/Exceptions/NotLeaderException.cs


View more (4)
4. src/KurrentDB.Client/Core/Exceptions/StreamTombstonedException.cs 🐞 Bug fix +2/-1

Replace null-forgiving with null-coalescing throw

• Replace GetRpcStatus()! with GetRpcStatus() ?? throw ex in FromRpcException factory method
• Ensures transport-level errors are not masked with NRE

src/KurrentDB.Client/Core/Exceptions/StreamTombstonedException.cs


5. src/KurrentDB.Client/Core/Exceptions/WrongExpectedVersionException.cs 🐞 Bug fix +2/-1

Replace null-forgiving with null-coalescing throw

• Replace GetRpcStatus()! with GetRpcStatus() ?? throw ex in FromRpcException factory method
• Prevents NRE when RpcException lacks Google.Rpc.Status details

src/KurrentDB.Client/Core/Exceptions/WrongExpectedVersionException.cs


6. src/KurrentDB.Client/Streams/KurrentDBClient.MultiAppend.cs 🐞 Bug fix +1/-2

Use null-conditional operator in MultiAppend catch

• Replace ex.GetRpcStatus()! with ex.GetRpcStatus()? using null-conditional operator
• Allows null status to fall through to default switch arm which rethrows original exception
• Prevents NullReferenceException on transport-level gRPC failures

src/KurrentDB.Client/Streams/KurrentDBClient.MultiAppend.cs


7. src/KurrentDB.Client/Streams/MaximumAppendSizeExceededException.cs 🐞 Bug fix +2/-1

Replace null-forgiving with null-coalescing throw

• Replace GetRpcStatus()! with GetRpcStatus() ?? throw ex in FromRpcException factory method
• Handles null status by throwing original RpcException

src/KurrentDB.Client/Streams/MaximumAppendSizeExceededException.cs


Grey Divider

Qodo Logo

@qodo-code-review
Copy link
Copy Markdown

qodo-code-review bot commented Apr 13, 2026

Code Review by Qodo

🐞 Bugs (1)   📘 Rule violations (0)   📎 Requirement gaps (0)   🖥 UI issues (0)   🎨 UX Issues (0)
🐞\ ◔ Observability (1)

Grey Divider


Remediation recommended

1. Rethrow resets stack trace 🐞
Description
MultiStreamAppendAsync rethrows the caught RpcException via throw <switchExpr> (default arm
returns ex), which resets the exception stack trace to this catch block instead of preserving the
original throw site. This makes transport-level failures harder to debug even though the original
RpcException type/message is preserved.
Code

src/KurrentDB.Client/Streams/KurrentDBClient.MultiAppend.cs[R82-85]

+				throw ex.GetRpcStatus()?.GetDetail<ErrorInfo>() switch {
					{ Reason: "STREAM_REVISION_CONFLICT" }         => WrongExpectedVersionException.FromRpcException(ex),
					{ Reason: "STREAM_TOMBSTONED" }                => StreamTombstonedException.FromRpcException(ex),
					{ Reason: "APPEND_RECORD_SIZE_EXCEEDED" }      => AppendRecordSizeExceededException.FromRpcException(ex),
Evidence
The catch block throws the result of a switch expression whose default arm evaluates to the caught
ex; throwing an exception variable/expression (as opposed to throw;) resets the stack trace to
the current throw location, obscuring the original gRPC failure site.

src/KurrentDB.Client/Streams/KurrentDBClient.MultiAppend.cs[81-89]
Best Practice: Microsoft Docs (.NET)

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`MultiStreamAppendAsync` currently rethrows the caught `RpcException` using `throw <expression>` where the expression may evaluate to `ex`. This resets the stack trace to the catch block, reducing debuggability for transport-level gRPC failures.

## Issue Context
This PR’s goal is to avoid masking transport failures (null `GetRpcStatus()`) and surface the original `RpcException`. Preserving the original stack trace aligns with that goal.

## Fix Focus Areas
- src/KurrentDB.Client/Streams/KurrentDBClient.MultiAppend.cs[81-89]

## Suggested approach
Rewrite the catch block to use statement-form control flow so the fallback path uses `throw;` (or `ExceptionDispatchInfo.Capture(ex).Throw()`) instead of `throw ex`:

- Compute `errorInfo = ex.GetRpcStatus()?.GetDetail<ErrorInfo>()`
- If `errorInfo?.Reason` matches a known reason, throw the typed exception
- Otherwise, `throw;` to rethrow the original exception with its stack trace preserved

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a crash path in the client’s gRPC error handling where RpcException.GetRpcStatus() can be null (transport-level failures), previously causing NullReferenceException and masking the original RpcException.

Changes:

  • Update MultiStreamAppendAsync to use null-conditional access when reading ErrorInfo from GetRpcStatus().
  • Update multiple FromRpcException factory methods to avoid null-forgiving GetRpcStatus()! and instead fall back to throwing the RpcException when no Google.Rpc.Status is present.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
src/KurrentDB.Client/Streams/KurrentDBClient.MultiAppend.cs Avoid NRE when GetRpcStatus() is null during multi-append error mapping.
src/KurrentDB.Client/Streams/MaximumAppendSizeExceededException.cs Guard against null GetRpcStatus() in exception factory method.
src/KurrentDB.Client/Core/Exceptions/WrongExpectedVersionException.cs Guard against null GetRpcStatus() in exception factory method.
src/KurrentDB.Client/Core/Exceptions/StreamTombstonedException.cs Guard against null GetRpcStatus() in exception factory method.
src/KurrentDB.Client/Core/Exceptions/NotLeaderException.cs Guard against null GetRpcStatus() in exception factory method.
src/KurrentDB.Client/Core/Exceptions/AppendTransactionMaxSizeExceededException.cs Guard against null GetRpcStatus() in exception factory method.
src/KurrentDB.Client/Core/Exceptions/AccessDeniedException.cs Guard against null GetRpcStatus() in exception factory method.
Comments suppressed due to low confidence (1)

src/KurrentDB.Client/Streams/KurrentDBClient.MultiAppend.cs:88

  • The default arm returns ex, which results in throw ex; and resets the original gRPC stack trace. Since this is inside a catch (RpcException ex), consider restructuring (e.g., assign mapped exception, and use throw; when no mapping applies) so transport-level failures truly rethrow the original exception without losing stack information.
					{ Reason: "STREAM_TOMBSTONED" }                => StreamTombstonedException.FromRpcException(ex),
					{ Reason: "APPEND_RECORD_SIZE_EXCEEDED" }      => AppendRecordSizeExceededException.FromRpcException(ex),
					{ Reason: "APPEND_TRANSACTION_SIZE_EXCEEDED" } => AppendTransactionMaxSizeExceededException.FromRpcException(ex),
					_                                              => ex
				};

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/KurrentDB.Client/Streams/MaximumAppendSizeExceededException.cs
Comment thread src/KurrentDB.Client/Streams/KurrentDBClient.MultiAppend.cs
Comment thread src/KurrentDB.Client/Core/Exceptions/StreamTombstonedException.cs
Comment thread src/KurrentDB.Client/Core/Exceptions/NotLeaderException.cs
Comment thread src/KurrentDB.Client/Core/Exceptions/AccessDeniedException.cs
@w1am w1am changed the title fix: handle null GetRpcStatus() to prevent NRE on transport-level gRPC errors fix: prevent NRE on transport level gRPC errors Apr 13, 2026
@w1am w1am merged commit ce48e55 into master Apr 14, 2026
153 of 155 checks passed
@w1am w1am deleted the fix/handle-null-rpc-status branch April 14, 2026 06:44
Copy link
Copy Markdown

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@w1am 👉 Created pull request targeting release/v1.3: #406

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants